test: the sanitizer subset must cover the C-level encoding selftest - #521
Conversation
test/run_san.sh runs a SUBSET of the suites, and a suite outside it is not
sanitized -- silently, because nothing reports the omission.
encode_invariants was outside it. That is the only suite that drives
pgcolumnar_debug_encoding_selftest, which exercises bitunpack at every width
1..64 across counts 1,2,3,7,8,9,17,64,129 plus a derived count per width.
It matters because it is the only fixture that crosses bitunpack's fast/tail
boundary in both directions. nFast is zero until the encoded body reaches nine
bytes (n * width >= 65), so small counts run the tail and larger ones the wide
load. Measured with a probe build that recorded whether the tail loop executed,
counting backends that reached it:
encode_invariants 21
differential 0
differential's chunks are large enough that nFast == n throughout. So the
sanitizer pass covered one of bitunpack's two paths, over exactly the code commandprompt#514
rewrote, and "clean under ASAN" described the half that ran.
Adding encode_invariants to the default subset closes that. The suite is already
clean there: 11 pass / 0 fail with no sanitizer reports under the ASAN+UBSAN
build.
## Tests
The check asks whether every suite driving the selftest is in the subset, rather
than naming encode_invariants, so moving the selftest elsewhere cannot quietly
narrow it. Two premises first, because it has two ways to pass vacuously -- an
empty subset parse, or no drivers found at all:
PASS premise: run_san.sh's default subset was found and is non-empty
PASS premise: at least one suite drives the C-level encoding selftest
Red before the change, and again with run_san.sh alone reverted:
FAIL the sanitizer subset runs every suite that drives the encoding selftest: got [encode_invariants] want []
The sweep skips harness_selftest.sh itself: it names the function in the pattern
it searches with, so a blind sweep matches the searcher as well as the searched.
That is the same self-match that makes `pgrep -f <pattern>` find its own command
line, and the first run of this check reported [encode_invariants harness_selftest].
Refs commandprompt#520, commandprompt#514.
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
Approved. This closes the gap I raised this morning and closes it better than I asked: asking "every suite that drives the selftest" rather than naming encode_invariants means relocating the selftest cannot quietly narrow the check. Same construction as deriving the oracle's count instead of listing it.
The 21-against-0 probe is the part that makes the case, and it is measured rather than argued: differential's chunks are large enough that nFast == n throughout, so a sanitizer pass including it and not encode_invariants covers one of bitunpack's two paths — over exactly the code #514 rewrote. "Clean under ASAN" described the half that ran.
Skipping your own file to avoid the self-match, with the pgrep -f lesson cited in the comment, is the right instinct applied before it bit rather than after.
Two non-blocking notes, both about what the check can and cannot see
1. It validates the DEFAULT subset, not the effective one. SUITES is ${PGC_SAN_SUITES:-...}, so a run with that variable set is sanitized over a subset this check never inspected. That is fine for the property you are guarding — the shipped default must cover the selftest — but the check's name reads as the stronger claim. One line saying "the default; an override is the operator's business" would fix the reading.
2. This parses the source, and the repository has already replaced that pattern once. CONTEXT.md:169 says it directly:
Count suites by asking the runner, never by parsing the source:
bash test/run_all_versions.sh --list-suites. A text parser over the array disagrees with bash on exactly the mistake this invites, and the disagreement is silent.
That is #473's lesson, and run_san.sh has no --list-suites to ask. Today the block is comment-free so the parse is exact and there is no live defect. But the failure direction matters: a suite name appearing in a comment inside the SUITES= block would satisfy grep -qw and produce a false PASS — the dangerous direction, since it asserts coverage that does not exist. The safe direction (an incomplete parse producing a false FAIL) is loud and self-correcting.
Giving run_san.sh the same --list-suites treatment run_all_versions.sh got would make this exact rather than currently-exact. Not for this PR; worth a follow-up, and I would take it if you would rather not.
Neither note weakens what this lands. Merging.
|
Correcting my own review: the hazard I described in point 2 does not exist, and I only found that out by trying to reproduce it. I said a suite named in a comment inside the The reason is simple once seen. What survives is the weaker and sufficient reason, which is the one #473 and And the fix I was about to ship for it would have introduced a real defect. The version on Nothing here weakens #521, which is already merged and correct. I am recording it because a review note that names a specific failure mode carries the same weight as a finding, and mine was wrong — and because the fix it prompted was a regression until the override was cleared. |
test: ask run_san.sh for its subset, with the override cleared (#521 follow-up)
2 participants
test/run_san.shruns a subset of the suites, and a suite outside that subset is not sanitized — silently, because nothing reports the omission.encode_invariantswas outside it.Why that one matters
It is the only suite that drives
pgcolumnar_debug_encoding_selftest, which exercisesbitunpackat every width 1..64 across counts 1, 2, 3, 7, 8, 9, 17, 64, 129 plus a derived count per width.That makes it the only fixture crossing
bitunpack's fast/tail boundary in both directions:nFastis zero until the encoded body reaches nine bytes (n * width >= 65), so small counts run the tail and larger ones the wide load. Measured with a probe build that recorded whether the tail loop executed, counting backends that reached it:encode_invariantsdifferentialdifferentialis in the subset; its chunks are large enough thatnFast == nthroughout. So the sanitizer pass covered one ofbitunpack's two paths — over exactly the code #514 rewrote — and "clean under ASAN" described the half that ran.Tests
The check asks whether every suite driving the selftest is in the subset, rather than naming
encode_invariants, so relocating the selftest cannot quietly narrow it. Same construction as the oracle deriving its count instead of listing it.Two premises first, because the check has two ways to pass vacuously — an empty subset parse, or no drivers found at all:
Red before the change, and again with
run_san.shalone reverted (the removal proof):The sweep skips
harness_selftest.shitself. It names the function in the pattern it searches with, so a blind sweep matches the searcher as well as the searched — the same self-match that makespgrep -f <pattern>find its own command line. The first run of this check reported[encode_invariants harness_selftest].Gate
harness_selftest54 checks / 0 fail, 0 warnings each.run_san.shitself, with its own fatal-on-violation options rather than options I chose:PASS encode_invariants, 1 suite under sanitizers, 0 failed, 0 ran no checks. Adding a suite to the subset would be worthless if it failed there.Refs #520, #514.